Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xds: Add passive health check config for upstreams (aka envoy outlier detection) #7713

Merged
merged 2 commits into from
May 8, 2020
Merged

xds: Add passive health check config for upstreams (aka envoy outlier detection) #7713

merged 2 commits into from
May 8, 2020

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Apr 27, 2020 •
edited
Loading

Closes #6832

Adds support for configuring two of the many outlier detection settings which provide passive health checking. These two configuration options are approximately the two options supported by HAProxy and NGinx.

The new configuration is being added to proxy.upstreams[].config. Envoy has a number of settings (enabled by split_external_local_origin_errors) that change the behaviour for local vs remote errors. I think if we were to configure these passive health check settings from the proxy config of the destination service (instead of the upstream config of the source service), it may be difficult to conceptualize "local".

I am opening this PR with a couple of open TODOs to get feedback on the naming and location of the config (UpstreamConfig).

TODO

  • docs
  • some coverage of the new config from the envoy integration suite

@crhino
Copy link
Contributor

crhino commented Apr 28, 2020

I think the naming seems reasonable to me. passive_health_check threw me off for a bit until I read through Envoy and Nginx docs and realized they both referred to these types of checks as passive.

max_failures also seems reasonable, especially since I don't see a compelling reason to enable split_external_local_origin_errors.

mkeeler
mkeeler reviewed Apr 29, 2020
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

It should just work with all the various Proxy config merging but it would be good when going through the integration tests to ensure that setting these in various places trickles down properly.

  • proxy-defaults/global config entry
  • service-defaults/<service name>
  • In the individual proxy configuration: Upstream.Config.

@dnephin dnephin force-pushed the dnephin/connect-proxy-passive-healthcheck branch from fb7df09 to 88b0196 Compare April 30, 2020 20:18
@preetapan preetapan added this to the 1.8.0 milestone May 4, 2020
@dnephin dnephin force-pushed the dnephin/connect-proxy-passive-healthcheck branch from 88b0196 to a08f0a2 Compare May 5, 2020 20:17
@dnephin
Copy link
Contributor Author

dnephin commented May 5, 2020

I added some passive_health_check config to one of the integration tests. The max_failures field is working, but interval is not for some reason. I'll continue to debug. Pushed a commit to see if it is broken on all versions of envoy.

@dnephin dnephin force-pushed the dnephin/connect-proxy-passive-healthcheck branch from a08f0a2 to 0861a27 Compare May 6, 2020 00:22
@dnephin
Copy link
Contributor Author

dnephin commented May 6, 2020

Tests are passing! This is ready for a full review now.

@dnephin dnephin force-pushed the dnephin/connect-proxy-passive-healthcheck branch from 0861a27 to 043d4d1 Compare May 6, 2020 16:24
crhino
crhino reviewed May 8, 2020
View reviewed changes
website/pages/docs/connect/proxies/envoy.mdx Outdated
@@ -277,6 +277,17 @@ definition](/docs/connect/registration/service-registration) or
since HTTP/2 has many requests per connection. For this configuration to be
respected, a L7 protocol must be defined in the `protocol` field.

- `passive_health_check` - Settings for the load balancer, used to remove
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `passive_health_check` - Settings for the load balancer, used to remove
- `passive_health_check` - Settings for the proxy, used to remove

nit, but I would use the word proxy instead of load balancer so as not to introduce another noun into the mix

crhino
crhino reviewed May 8, 2020
View reviewed changes
Copy link
Contributor

@crhino crhino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small comments, but nothing blocking or big, good work! Just worked when I tested it out. 👍

@dnephin dnephin force-pushed the dnephin/connect-proxy-passive-healthcheck branch from 043d4d1 to 5655d7f Compare May 8, 2020 18:57
@dnephin dnephin mentioned this pull request May 8, 2020
Closed
crhino
crhino approved these changes May 8, 2020
View reviewed changes
Copy link
Contributor

@crhino crhino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@dnephin dnephin merged commit c4ad843 into master May 8, 2020
@dnephin dnephin deleted the dnephin/connect-proxy-passive-healthcheck branch May 8, 2020 19:48
@dependabot dependabot bot mentioned this pull request Mar 14, 2021
Closed
@fredwangwang fredwangwang mentioned this pull request Apr 9, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkeeler mkeeler mkeeler left review comments

@crhino crhino crhino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.

connect: allow configuration of "outlier detection"
4 participants
@dnephin @crhino @mkeeler @preetapan